Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RC_Channel: Aux switches to respect 'reverse' option #13172

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

SergeyBokhantsev
Copy link
Contributor

No description provided.

@tridge
Copy link
Contributor

tridge commented Jan 4, 2020

we probably should have done this when we first introduced the feature. Unfortunately if we apply this now then it could be dangerous. eg. someone may have a RC option setup for arming and the vehicle may arm when they upgrade firmware.
@peterbarker, what do you think?

@IamPete1
Copy link
Member

IamPete1 commented Jan 5, 2020

I think this is useful to have, we could go through and set all switch reversing params to zero as part of this. It should be easy to tell switch from not switch once this is in. #8307

@rmackay9
Copy link
Contributor

rmackay9 commented Jan 6, 2020

We also don't use the RCx_MIN/MAX or _TRIM values and a similar issue exists for the flight mode switch.

My feeling at the moment is that this change, while certainly valid if we'd done it originally, will cause more problem than it will resolve if we introduce it now.

@peterbarker
Copy link
Contributor

As @tridge points out, there will be transition issues.

@IamPete1 suggests using configured_in_storage - but someone may have explicitly set those parameters themselves - and we currently ignore them but would stop, leading to potential arming attempts.

@rmackay9 points out we don't use the min/max/trim parameters; it's unclear to me how we would do that since AFAIK aux and mode switches have always been done on raw PWM threshold values. Would be a rather interesting documentation problem, if nothing else, if you were to attempt to apply the parameters to the mode switch!

What problem are we attempting to solve here? I'm guessing some sort of non-programmable transmitter?!

@SergeyBokhantsev
Copy link
Contributor Author

@peterbarker the problem is the same as one solved by having 'reverse' option for channel value, I believe. It looks like there is no way to safe handle this change, so it is easier leave it as is.
I'm not forcing this in any way.

@tridge
Copy link
Contributor

tridge commented Jan 6, 2020

we could add a bit in RC_OPTIONS which says "honor the reversed flag on switches"

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this conditional on a new RC_OPTIONS bit to enable this

@SergeyBokhantsev SergeyBokhantsev force-pushed the RC_Channel_aux_reverse branch 2 times, most recently from 2a95ecf to 4cac1d4 Compare January 9, 2020 12:24
@SergeyBokhantsev
Copy link
Contributor Author

@tridge done with RC_OPTIONS

@IamPete1
Copy link
Member

IamPete1 commented Jun 7, 2020

@SergeyBokhantsev Thanks for updating this, sorry it has taken us so long to get to it. It would be great if you could rebase it.

@tridge
Copy link
Contributor

tridge commented Jun 7, 2020

I have rebased this

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to take this to the devcall?

@peterbarker
Copy link
Contributor

Ah. I just had a thought.

Some options are considered "high" when they're in the middle. We should consider how that's handled when the switch is reversed.

@peterbarker
Copy link
Contributor

avoidance and OSD control are two examples where behaviour isn't strictly reversed by this flag.

@andyp1per
Copy link
Collaborator

I think for OSD control it's only actual switches where this is true - the stick commands use high/low. So I think it's fine.

@SergeyBokhantsev
Copy link
Contributor Author

Mid-position behavior does not change with or without the reverse. I believe this is ok. How it may be a problem?

@@ -358,6 +358,11 @@ class RC_Channels {
return get_singleton() != nullptr && (_options & uint32_t(Option::FPORT_PAD));
}

// should a channel reverse option affect aux switches
bool switch_reverse_allowed(void) const {
return get_singleton() != nullptr && (_options & uint32_t(Option::ALLOW_SWITCH_REV));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can remove the get_singleton() check on all these functions, it looks like it was leftover from when we needed singleton to get at _options

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the point was that the number of positions that enable a feature stays the same. Not sure if that's natural for "reversal" or not, but others seem to think so.

@tridge tridge merged commit f92d539 into ArduPilot:master Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants